[#993] Register an entry container's configuration listeners only once it has opened - #999

praise: The PR closes the leak it names and proves it: four of the five cases are red at BASE exactly as the table says (run here: tests=5 failures=4), and the fifth kills the storageOpened-forced-true mutant.

• The replay give-back at the top of openAndRegisterEntryContainers closes the ERR_ENTRY_CONTAINER_ALREADY_REGISTERED road traced on #883, and aReplayedOpenLeavesOneSetOfEntryContainers pins it both ways: delete the loop → ALREADY_REGISTERED; unregister without close → stillRegisteredOn(cfg) non-empty.
• catch (Exception e) { close(); throw e; } is pinned by aBackendWhichFailsToOpenLeavesNothingRegistered through the index listener, and EntryContainer.close() has no throwing path (every callee read), so the original exception is the one that escapes.
• The issue analysis corrected the report — not an NPE, more than five listeners — before the fix was written.

issue (non-blocking): The entry-container loop in giveUpAfterFailedOpen is pinned by no case — deleting it is green 5/5.

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:191-194

Every fixture has one base DN and openEntryContainer runs before registerEntryContainer, so the failing container is never in entryContainers: tests 1-3 iterate an empty map, test 4 fails in storage.open, test 5 succeeds. Measured: the loop removed, FailedBackendOpenTest passes 5/5. The road it exists for — first base DN registered, the second's open fails — is untested. RefusingOneTree matches on getIndexId() alone, so it cannot single out the second container's tree; match on the full name.

// TrackingStorage / RefusingOneTree: refuse by full tree name, not index id alone
if (refused.equals(name.toString()))          // "/dc=com,dc=b993b/id2entry"

@Test
public void aSecondBaseDNWhichFailsToOpenGivesBackTheFirst() throws Exception
{
  final TrackedBackend backend = new TrackedBackend();
  backend.setBackendID(BACKEND_ID);
  final PDBBackendCfg cfg = backendCfg(newTreeSet(BASE_DN, DN.valueOf("dc=b993b,dc=com")));
  backend.configureBackend(cfg, serverContext);
  backend.storage.removeStorageFiles();
  backend.storage.failOpeningTree("/dc=com,dc=b993b/id2entry");
  try
  {
    backend.openBackend();
    fail("the backend was expected not to open with a second base DN whose trees cannot be opened");
  }
  catch (InitializationException expected)
  {
  }
  finally
  {
    backend.storage.close();
  }
  assertThat(stillRegisteredOn(cfg)).isEmpty();        // the first container's five
  assertThat(stillRegisteredOn(indexCfg)).isEmpty();   // and its cn index
}

Pin: with the loop deleted this case must go red on stillRegisteredOn(cfg).

issue (non-blocking): A storage.open() which throws leaves the PDB quota, the PDB change listener and a half-built Persistit behind, and the reason the javadoc gives for not closing it is not the real one.

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:137-138, :182-185, :196-199
opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1060, :1085-1095, :1172-1199, :1103-1119

Before open0 can throw, PDBStorage has already registered itself as a PDB change listener (constructor, :1060), acquired dbCacheSize from the MemoryQuota (buildConfiguration, :1090/:1095) and assigned db = new Persistit(dbCfg) (:1182); the catches at :1193-1199 only rewrap. With storageOpened == false nothing gives that back, and RootContainer cannot "always close": PDBStorage.close() with db != null and monitor == null NPEs in DirectoryServer.deregisterMonitorProvider(null) (:1107) before db.close() and the quota release. Every failed enable of a PDB backend whose directory is unwritable, whose volume is corrupt or in use drains one dbCacheSize for the JVM's life and leaves a storage answering PDB config changes — #993 one class down. Pre-existing (BASE closed nothing on any failed-open road); JDBCStorage.open self-unwinds (:1059-1076).

The javadoc's rationale — "a read only root container is opened over the very storage instance the backend holds" — is a road that does not exist: every READ_ONLY open (BackendImpl.exportLDIF:620, verifyBackend:727, rebuildBackend:772, importLDIF:685) is gated on mustOpenRootContainer() i.e. rootContainer == null, so no live root container holds the instance, storage.open() genuinely opens it, and closing it on failure is correct. The false arm fires on exactly one road: storage.open() threw.

// PDBStorage.open: give back what buildConfiguration/open0 took before throwing
@Override
public void open(AccessMode accessMode) throws ConfigException, StorageRuntimeException
{
  Reject.ifNull(accessMode, "accessMode must not be null");
  if (isBackendIncomplete(accessMode))
  {
    return;
  }
  boolean opened = false;
  try
  {
    open0(buildConfiguration(accessMode));
    opened = true;
  }
  finally
  {
    if (!opened)
    {
      giveUpFailedOpen();   // db.close() best-effort + db = null; release memQuota; removePDBChangeListener(this)
    }
  }
}

Or: file it as a follow-up against PDBStorage — but in either case rewrite the storageOpened rationale in the giveUpAfterFailedOpen javadoc, the PR body, test 4's javadoc and comment (FailedBackendOpenTest.java:209-211, :229) and the mock message (:474) to the true one: a storage whose open() threw is not one this call can close, and what it took is the storage's own to give back.

issue (non-blocking): A container which opened but fails before registerEntryContainer is reclaimed by nobody.

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:288-290

After openEntryContainer returns, the container has registered its five listeners plus one per index; ec.getHighestEntryID(txn) runs before registerEntryContainer(baseDN, ec). A StorageRuntimeException from that cursor read (Persistit.getExchange / Exchange.previous: TreeNotFoundException, PersistitIOException, CorruptVolumeException, TimeoutException) leaves ec a local: giveUpAfterFailedOpen and the replay loop walk entryContainers only, and EntryContainer.open's catch has returned. Not replayed — a fetch never raises RollbackException (both throw sites in Exchange.java are on the store path) — so one leaked set per attempt, and the server continues. Pre-existing window; the PR narrows every other road and leaves this one.

EntryContainer ec = openEntryContainer(baseDN, txn, accessMode);
registerEntryContainer(baseDN, ec);            // held before anything else here can throw
EntryID id = ec.getHighestEntryID(txn);

suggestion (non-blocking): No case asserts that an opened container is registered — the five registrations can be deleted at their new site and the class stays green.

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java:561-566
opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java:281-333

Measured: the five registrations moved back to the constructor, everything else at head — 5/5 green, because catch (Exception) { close(); } removes what the constructor added. That revert is unkillable by construction (constructor and open() are always paired), but the delete at the new site is pinnable, and stillRegisteredOn uses List.removeAll, which hides a double registration.

// positive twin, after a successful openBackend()
backend.openBackend();
final List<Object> registered = stillRegisteredOn(cfg);
assertThat(registered).filteredOn(l -> l instanceof EntryContainer).hasSize(1);
// the two private cfg managers, each once as add and once as delete listener: 4, not 0 and not 8
assertThat(registered).filteredOn(l -> l.getClass().getSimpleName().endsWith("IndexCfgManager")).hasSize(4);

// stillRegisteredOn: remove one occurrence per removal, so a double registration is visible
for (Object removed : changeRemoved.getAllValues())
{
  registered.remove(removed);
}

suggestion (if-minor): aRootContainerWhichCouldNotOpenTheStorageDoesNotCloseIt asserts closeCalls() == 0 only; the root container's own listener removal on the storageOpened == false arm is unpinned.

opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java:232

An early return when !storageOpened at the top of giveUpAfterFailedOpen is green 5/5 by reading: the other four cases have storageOpened == true.

assertThat(backend.storage.closeCalls()).isEqualTo(0);
assertThat(stillRegisteredOn(cfg)).isEmpty();

praise: The failed-open give-back now holds on every road the PR names, and each fix is pinned by a case that goes red without it.

• registerEntryContainer before getHighestEntryID (RootContainer.java:291-297): reverting the order turns anEntryContainerWhichOpenedButWasNotRegisteredIsGivenBack red (1/8).
• The give-back loop and the storageOpened arm of giveUpAfterFailedOpen (RootContainer.java:189-205): loop deleted → 2/8 red; early return on !storageOpened → 1/8 red.
• PDBStorage.openOrGiveBack with rejectIfOpen ahead of buildConfiguration (PDBStorage.java:1159, :1182-1204), pinned by the three new PDBStorageTest cases; the memQuota = null comment (:1133-1135) names the import road's extra close, so the 3 → 2 release rebalance reads as intended.
• CI green at this head on the Linux cells, where the failsafe suite runs.

question: Does the storage give-back stop at PDBStorage on purpose? JEStorage.open still leaks the quota and its change listener when open0 throws.

opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:829-840, :719, :764-771, :866-873, :786-822; opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:182-187

open() runs buildConfiguration (quota acquired at :764-771) before open0; the double-open guard sits inside open0 (:866-869), after the quota was taken; new Environment(...) (:871) throwing leaves the quota taken and the ctor-registered listener (:719) in place, and nobody calls close() — giveUpAfterFailedOpen(false) closes no storage by design. close() (:786-822) never nulls memQuota, so a second close releases twice. This is PDBStorage's shape at BASE; the PR body claims the give-back generally, the RootContainer javadoc names PDBStorage and JDBCStorage only. Pre-existing and untouched by the PR — a follow-up issue named in that javadoc is fine if the scope is deliberate; otherwise the PDBStorage moves mirror:

// JEStorage.open — refuse before taking; give back what a failed open0 took
if (env != null)
{
  throw new IllegalStateException(
      "Database is already open, either the backend is enabled or an import is currently running.");
}
buildConfiguration(accessMode, false);
boolean opened = false;
try
{
  open0();
  opened = true;
}
finally
{
  if (!opened)
  {
    try { close(); } catch (RuntimeException e) { logger.traceException(e); }
  }
}

// JEStorage.close — release once: the import road closes twice
memQuota.releaseMemory(...);
memQuota = null;

Keep the guard inside open0 as well — its :903 caller does not come through open().

Pin: the PDBStorageTest.aStorageWhoseOpenFailedGivesBackWhatItTook shape on a JEStorage over a locked directory — quota back to availableBefore, verify(cfg).removeJEChangeListener(second).

issue (non-blocking): A failed-open case whose failure does not come leaves the backend's base DN registered, and the next case dies in openBackend() instead of on its own assertion.

opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/FailedBackendOpenTest.java:286-315, :331

Measured: with the five registrations deleted from the end of EntryContainer.open(), anEntryContainerWhichOpenedButWasNotRegisteredIsGivenBack opens where it expected to fail; its finally closes the storage (:309) but not the backend, and anEntryContainerWhichOpenedIsRegisteredOnce went red on base DN dc=b993,dc=com already registered — its hasSize(1) (:346) was never reached (2/8 red, the second one for the wrong reason). The twin also calls openBackend() (:331) outside its try, where aReplayedOpenLeavesOneSetOfEntryContainers (:369-378) guards the same call.

boolean opened = false;
try
{
  backend.openBackend();
  opened = true;
  fail("the backend was expected not to open when the highest entry ID cannot be read");
}
catch (InitializationException expected)
{
  // What a storage which cannot position a cursor on the last entry does.
}
finally
{
  if (opened) { backend.closeBackend(); } else { backend.storage.close(); }
}

The same finally around :331.

issue (non-blocking): PDBStorage.close() skips the quota, listener and disk-monitor releases when db.close() throws.

opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1113-1142

A PersistitException from db.close() (:1115) becomes the IllegalStateException at :1120 and leaves :1123-1142 unrun; on the give-back road openOrGiveBack's catch (:1201) traces it and the quota and the listener stay taken — the round-1 leak, one road narrower. Reachable only with db != null on that road (an Error after :1230), so defensive: the releases ahead of the db.close() block, or a finally.

@Override
public void close()
{
  if (memQuota != null) { /* the :1123-1136 block, memQuota = null included */ }
  config.removePDBChangeListener(this);
  if (diskMonitor != null)
  {
    diskMonitor.deregisterMonitoredDirectory(getDirectory(), this);
  }
  if (db != null) { /* :1105-1121, unchanged */ }
}

suggestion (non-blocking): The db != null arm of the give-back (monitor != null guard, db.close()) and the openOrGiveBack swallow run in no case.

opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1105-1120, :1201

The three new PDBStorageTest cases fail the open at new Persistit(dbCfg) (:1230), so db is null when close() runs; FailedBackendOpenTest case 4 throws in the TrackedStorage wrapper before the delegate's open. Past :1230 nothing in open0 throws in production (loadVolume is a lookup of a configured volume), so deleting the monitor != null guard survives every case by construction. Either document the arm as defensive, or pin it with an injected failure past :1230 — the disk monitor is already a mock (PDBStorageTest.java:75):

@Test
public void aStorageWhoseOpenFailedAfterItsDatabaseOpenedGivesTheVolumeBack() throws Exception
{
  final DiskSpaceMonitor refusing = serverContext.getDiskSpaceMonitor();
  doThrow(new RuntimeException("disk monitor refused")).when(refusing).registerMonitoredDirectory(any(), any());
  final PDBStorage second = new PDBStorage(createBackendCfg(), serverContext);   // over a second directory
  try
  {
    second.open(AccessMode.READ_WRITE);
    fail("the storage was expected not to open when its directory cannot be monitored");
  }
  catch (RuntimeException expected)
  {
    // What the failure past the database open does.
  }
  reset(refusing);
  // The volume was given back: a third storage over the same directory opens.
  final PDBStorage third = new PDBStorage(createBackendCfg(), serverContext);
  third.open(AccessMode.READ_WRITE);
  third.close();
}

thought (non-blocking): A live db-cache-size change drifts the quota: open acquires the size at open (:1087-1095), close releases the size at close (:1125-1131), and applyConfigurationChange swaps config (:1637) without touching memQuota.

opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1637, :1087-1095, :1125-1131

Pre-existing and outside this PR's lines — noted for a follow-up, no action here.